[Icons] Do not fail application if there is not internet connection#3146
[Icons] Do not fail application if there is not internet connection#3146Zales0123 wants to merge 1 commit intosymfony:2.xfrom
Conversation
|
Hi Mateusz, by any chance were you at the Sylius Con in Lyon this last friday? :) About your changes, I already faced the same issue when working on a train/plane, where the connection was really bad and icons were not locked (ie #3084). |
There was a problem hiding this comment.
CI failures seem unrelated
EDIT: posted at zenstruck/foundry#1011 (comment)
| } catch (TransportException $e) { | ||
| $this->logger?->warning($e->getMessage()); | ||
|
|
||
| return ''; | ||
| } |
There was a problem hiding this comment.
We could merge this block with the catch (IconNotFoundException $e)
There was a problem hiding this comment.
The question is - should it be handled only if the ignoreNotFound flag is set to true, or it should work this way no matter what 🤔
There was a problem hiding this comment.
should it be handled only if the
ignoreNotFoundflag is set to true
IMO, yes
There was a problem hiding this comment.
Up to you, @Kocal 🖖 From my perspetive, a decision of not failing on not found icons is a consious choice: "if I define an unknown icon it should not fail" and in fact is an opt-in. On the other hand, not failing the application when something completely independent of the developer happens (like the internet connection problems) should be a default behaviour (or maybe opt-out 🤷).
Anyway, if you decide to cover this with the existing flag-based approach, I'm ok with that 🫡 but maybe in such a situation we should indeed just handle the TransportException in IconifyOnDemandRegistry and throw IconNotFoundException so no changes in the runtime service is needed. Again - up to you, I'm just a humble developer that found a bug 😄
There was a problem hiding this comment.
In my opinion I find it ridiculous that an app could returns a 500 if a single icon was not able to be downloaded on-the-fly (implying the icon was not imported/locked during development or deployment).
Your addition is nice (we still need to merge the 2 catch), however I think the DX could be improved.
I wasn't aware about this option, and I think some people too, but IMHO it would be nice to add a new Symfony Recipe for UX Icon with:
- the option set to
falsetrue, so we make it explicit - the option set to
truefalseinwhen@dev
Do you want to work on it? I can help you if you need.
Thanks!
There was a problem hiding this comment.
the option set to false, so we make it explicit
the option set to true in when@dev
IMO, ignoreNotFound should be true to prevent production to return a 500 when an icon is missing.
And false in when@dev
This is why that option was originally added: #2008
There was a problem hiding this comment.
Ah, yes indeed, I forgot what was the option name, but yes these errors should be silent in prod. I updated my message.
carsonbot
added
Status: Reviewed
Indeed I was there 😅
Of course I had the bad luck opening the PR just after some dependencies updates break the build 😆 |
Kocal
added
Status: Needs Work
|
Gave my reasons to not silent per default (we can change the receipe if you want !) And i'm a bit surprised @Kocal tbh ...my memory is maybe incorrect, but i remember you pushing hard to trigger throwable in prod for AssetMapper ... 🤔 Anyway, to improve DX here, another thing could be to add some class or default icon when one is not found ? |
smnandre
left a comment
smnandre
left a comment
There was a problem hiding this comment.
(Let's keep the current behaviour to prevent BC break, and improve DX with some catch/throw indeed when the flag is on, and set this flag default in next major? )
Are you referring to symfony/recipes#1347 maybe? 😛 tl;dr: when developing, the AssetMapper was not able to resolve an asset. I didn't see a warning message was logged, I expected an exception to be thrown. However in production, a warning message instead of an exception is the perfect behavior (like To resume the PR, I think we all want:
ux_icons:
# do not silence exceptions during rendering if icon is not found
ignore_not_found: false
when@prod:
ux_icons:
ignore_not_found: trueNote that we can't directly use |
|
Opened symfony/recipes#1477 |
|
You defined the width/height at the same time 🤝 |
|
The recipe has been merged |
5 participants
Hello 👋 First of all, thank you for amazing SymfonyUX initiative, it definitely brings a frash air in working with front-end for a total FE noobs like me 😅
I've recently encountered a rather unusual problem while playing in icons, still a pretty annoying one. I was coding in a plane and for some reason decided clear my cache. It resulted in a situation, when I had no icons information loaded in cache and they could not be fetched from the remote server as I had no internet connection✈️ It resulted in 500 error and not being able to use my application just because of the missing icons 😅 I believe such a situation should be handled by default and unablity to show icons should not break the application.
I propose the easiest possible solution - catching the
TransportExceptionon the high level in theUXIconRuntime. However, I suppose it could be done nicer, to not expose any registry specific information (in the end, only theIconifyOnDemandRegistrycould result in such error) - maybe we should handle it in the registry itself, and throw some more generic exception, likeIconsNotAvailableException? Or maybe even use the existingIconNotFoundException? 🤔 I didn't want to push any of these solutions, and would prefer to see the opinion of the library maintainers 🫡